-
-
Notifications
You must be signed in to change notification settings - Fork 398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow changing coefficient type in read_from_file #3801
Conversation
This is also going to require a bump in the minimum MOI compat bound |
So I realized we already support this: julia> using JuMP
julia> file = joinpath(dirname(dirname(pathof(MOI))), "test", "FileFormats", "SDPA", "models", "example_A.dat-s")
"/Users/oscar/.julia/packages/MathOptInterface/2CULs/test/FileFormats/SDPA/models/example_A.dat-s"
julia> model = open(file, "r") do io
return read(
io,
GenericModel{BigFloat};
format = MOI.FileFormats.FORMAT_SDPA,
number_type = BigFloat,
)
end
A JuMP Model
Minimization problem with:
Variables: 2
Objective function type: GenericAffExpr{BigFloat, GenericVariableRef{BigFloat}}
`Vector{GenericAffExpr{BigFloat, GenericVariableRef{BigFloat}}}`-in-`MathOptInterface.PositiveSemidefiniteConeTriangle`: 2 constraints
Model mode: AUTOMATIC
CachingOptimizer state: NO_OPTIMIZER
Solver name: No optimizer attached. Perhaps it just needs documenting. Reading non-Float64 file formats is pretty niche. |
I don't think we should do this. As yet, most file formats do NOT support reading coefficients other than Float64. |
Oh indeed, I forgot about that keyword argument of |
It's not sufficient just to create a |
Oh, you've removed the kwarg here. |
I assume that if a file format allow specifying the type with a keyword argument then it will have been tested for different types as well. |
Yeah yeah I thought we were still having the two separate kwargs. One at the MOI level and one at the JuMP level. Disregard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But it needs a test.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3801 +/- ##
==========================================
+ Coverage 98.38% 98.40% +0.02%
==========================================
Files 44 44
Lines 5879 5968 +89
==========================================
+ Hits 5784 5873 +89
Misses 95 95 ☔ View full report in Codecov by Sentry. |
Before this PR
After this PR:
I'm a bit hesitant between calling this keyword argument
coefficient_type
(likeJuMP.add_bridge
) orvalue_type
. Foradd_bridge
, IIRC, we chosecoefficient_type
because it didn't really have to match with the model value type. In jump-dev/MathOptInterface.jl#2532,value_type
as a keyword could be weird becausevalue
is out of scope there and we could say that in this PR we usecoefficient_type
to be consistent withMOI.FileFormats.Model
.Requires